-
Notifications
You must be signed in to change notification settings - Fork 500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
panic runtime and C-unwind documentation #1226
base: master
Are you sure you want to change the base?
Conversation
Hm... not sure how to fix the links to the newly-introduced page. Is there an index page I need to edit? Edit: I think I found it |
src/items/functions.md
Outdated
| panic runtime | ABI | `panic`-unwind | Unforced foreign unwind | | ||
| -------------- | ------------ | ------------------------------------- | ----------------------- | | ||
| `panic=unwind` | `"C-unwind"` | unwind | unwind | | ||
| `panic=unwind` | `"C"` | abort | UB | | ||
| `panic=abort` | `"C-unwind"` | `panic!` aborts | abort | | ||
| `panic=abort` | `"C"` | `panic!` aborts (no unwinding occurs) | UB | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| panic runtime | ABI | `panic`-unwind | Unforced foreign unwind | | |
| -------------- | ------------ | ------------------------------------- | ----------------------- | | |
| `panic=unwind` | `"C-unwind"` | unwind | unwind | | |
| `panic=unwind` | `"C"` | abort | UB | | |
| `panic=abort` | `"C-unwind"` | `panic!` aborts | abort | | |
| `panic=abort` | `"C"` | `panic!` aborts (no unwinding occurs) | UB | | |
| panic runtime | ABI | `panic`-unwind | Unforced foreign unwind | | |
| -------------- | ------------ | ------------------------------------- | ----------------------- | | |
| `panic=unwind` | `"C-unwind"` | unwind | unwind | | |
| `panic=unwind` | `"C"` | abort if unwinding reaches the function | UB if unwinding reaches the function | | |
| `panic=abort` | `"C-unwind"` | aborts immediately (no unwinding occurs) | abort if unwinding reaches the function | | |
| `panic=abort` | `"C"` | aborts immediately (no unwinding occurs) | UB if unwinding reaches the function | |
I found this a bit confusing. I believe there are subtle differences in terms of where the aborts occur and so forth. I have tried to clarify above, but I think it may be worth further clarifying.
It may also be worth adding some (perhaps non-normative) discussion of implementation:
- When compiling a function F with
panic=unwind
andextern "C"
, the compiler inserts unwinding guards for Rust panics that trigger an abort when unwinding reaches F.
I am also be misunderstanding what's going on. I was a bit surprised to see "UB" for unforced-foreign-unwind with C=unwind. I guess that this table is combining two scenarios:
- what happens when you call a C++ function declared as extern "C", and it unwinds (UB, we haven't compiled any guards)
- what happens when an
extern "C"
Rust function invokes some C++ function that throws (probably, in practice, an abort, but perhaps we have simplified to call it UB?)
Is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only UB for a foreign function declared as extern "C"
to unwind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nbdd0121 what happens when an extern "C"
Rust function unwinds? I believe we insert an abort guard, but this table doesn't clarify that, right? Or maybe I don't understand what it's trying to convey. I'm imagining a scenario like
extern "C-unwind" fn throws();
extern "C" fn rust_fn() {
throws(); // unwinds
}
In this case, I presume you get an abort -- and I think we guarantee that? But the way I read this table, it would be listed as UB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm....I don't know if the panic
abort guard would currently catch and abort in that case, or if it relies on the personality function to only abort on true Rust panic
s. I agree that the behavior in the table as-written is UB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nbdd0121 what happens when an
extern "C"
Rust function unwinds? I believe we insert an abort guard, but this table doesn't clarify that, right? Or maybe I don't understand what it's trying to convey. I'm imagining a scenario likeextern "C-unwind" fn throws(); extern "C" fn rust_fn() { throws(); // unwinds }In this case, I presume you get an abort -- and I think we guarantee that? But the way I read this table, it would be listed as UB.
Unwinding out from extern "C"
functions (defined in either Rust or foreign language) is UB.
In the case you listed, we insert guard to prevent unwinding from actually leaving a Rust extern "C"
functions, therefore the function does not unwind, so UB is prevented; in this case we never unwinds out from a extern "C"
Rust functions.
If you define a extern "C-unwind"
Rust function and transmute it to extern "C"
and then call it, it's not UB if unwinding does not happen, and it's UB if unwinding happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikomatsakis With the change to the verbiage above, explaining that the table entries are specifically describing behavior at function boundaries, do you still want to make a change here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check whether the notes I suggested to add under the table are correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikomatsakis Can I resolve this comment thread now?
Sorry for the delay; I think I've addressed all comments. |
@tmandry @nikomatsakis I'm not sure you saw my comments & changes last week, but I think this is ready for re-review. |
Could you squash the commits? |
@nbdd0121 Can that be done on merge? I've heard that GitHub sometimes has trouble with PR branches that receive force-pushes. |
I think I've resolved all open questions and concerns. Is there anything else needed from me at the moment? |
This really needs rebasing now. |
e8c62b4
to
6e83797
Compare
@nbdd0121 Done! |
@tmandry two changes since your review:
|
@chorman0773 Okay, please take a look now! |
47459f7
to
7965d3d
Compare
Sorry, I don't have time to figure out how to appropriately add rule identifiers to |
I can do the identifiers in panic.md tomorrow. I'll also take a look at the rest then. |
Co-authored-by: Daira-Emma Hopwood <daira@jacaranda.org>
src/linkage.md
Outdated
any `-unwind` ABI | ||
* The Rust crate containing the `-unwind` ABI declaration was compiled with | ||
`panic=unwind` | ||
* The final binary is linked with [the `panic=abort` runtime][panic-runtime] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You changed the meaning of the text by making this about the final binary. I am not sure whether this is correct. I'd prefer if you could just restore the old wording, which was AFAIK reviewed by a bunch of people, and leave further improvements to a later PR so that the diff over the previous consensus can be more easily reviewed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact I am fairly sure your new wording is wrong. If the unwinding comes from an external crate, enters through a -Cpanic=unwind crate, and then propagates into a -Cpanic=abort crate, that is UB, even if the binary is -Cpanic=unwind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We certainly talked about the original text a lot, but my impression was that you still considered it confusing. I started the change because it seemed related to the entirely-new "foreign linking" section, but seemed even more confusing in its original form when juxtaposed with the new section.
I believe the rule has always been about whether or not the "abort" runtime is linked, and since only one panic runtime can be linked in a final Rust binary, I don't understand how the introduction of the word "final" changes this. I can remove the word "final", though.
A panic=abort crate will have shims for aborting the process in the presence of unwinding at extern
boundaries. If there's no extern boundary, then the two crates are compiled as source code so they can't have different panic modes in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's no extern boundary, then the two crates are compiled as source code so they can't have different panic modes in the first place.
That's not right. You can link together different crates with different flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old text was:
No crate may be linked with [the `panic=abort` runtime][panic-runtime] if it has
both of the following characteristics:
* It contains a call to an `-unwind` foreign function or function pointer
* It was compiled with `panic=unwind`
Actually looking at this I think there is a mistake here -- the problematic case is linking a crate with the "unwind" runtime, but compiling it with panic=abort
, isn't it?
panic=abort
sets a bunch of "nounwind" flags, and ultimately what this here is all about is preventing those from causing UB. So for a concrete example:
- crate A is panic=unwind, calls some
C-unwind
function from a "Rust" ABI function - crate B is panic=abort, calls crate A and marks that call
nounwind
- now we have UB if that
C-unwind
call ever unwinds
It doesn't even matter which panic runtime is linked in, I think? This is about panics from the outside world after all.
The panic runtime would come in in a situation like this:
- crate A is panic=abort, calls e.g.
unwrap
and marks that call withnounwind
since it has the "Rust" ABI - the panic_unwind runtime is linked in
- now we have UB if that
unwrap
ever fails
Not sure where we say that that is UB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* The final binary is linked with [the `panic=abort` runtime][panic-runtime] | |
* The program unwinds into a rust crate compiled with `panic=abort` |
Would this be correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that sounds right. Cc @nbdd0121 to be sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But also, don't we have to say somewhere that if a crate is built with panic=abort, and later linked with the panic=unwind runtime, that's UB too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my view this is resolved by my later commits, but someone should take a look.
…or execution out of a note
I added the identifiers to the panic chapter. |
Should be ready other than the outstanding issue in linkage. |
src/linkage.md
Outdated
> **Note**: To protect against this undefined behavior, `rustc` does not permit | ||
> linking the `panic=abort` runtime against any crate that was compiled with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment also likely needs updating, since it's not about the panic=abort runtime, it's about mixing panic=abort and panic=unwind crates irrespective of which runtime is ultimately linked in.
src/linkage.md
Outdated
> library that may unwind. However, use of the `Rust` (default) ABI does not | ||
> cause a link-error, since that ABI is not expected to be used as an | ||
> entrypoint into a static or shared library. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get the last sentence.
Also, this should explain the gap in this rustc check. Clearly there's a gap, otherwise we wouldn't have to document this UB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UB is only possible when using a linker other than rustc (a "foreign linker" as described in the section above), so AFAIK there is no gap in rustc itself.
This sentence explains why the Rust ABI is not included in the lint, which is what you were originally asking about before I changed the verbiage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's existing text so I missed it. It should probably mention dlopen
-like situations? Even if not mixing Rust an non-Rust code, one may be using a non-rustc linker.
The entire section should then probably say:
If you are using a foreign linker, the following requirements must be upheld: [...]
And then the note can just say that when using rustc as a linker, those requirements are enforced automatically.
This sentence explains why the Rust ABI is not included in the lint, which is what you were originally asking about before I changed the verbiage.
This section isn't about the lint though? It's about a hard error on panic option mismatches. That one can't rely on what is "expected" to happen, it should be sound. Did the sentence end up in the wrong paragraph?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will have to wait for @BatmanAoD then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have pushed proposed wording to https://github.com/RalfJung/reference/commits/c-unwind-documentation/. Unfortunately I can't push to this PR.
(EDIT: I deleted that branch and added the commits in this PR as @BatmanAoD gave me access, thanks a lot. :)
I sure hope what that text says is correct, but it would be good if @BatmanAoD or @nbdd0121 could take a look. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have time right now to look, but I've added both you and Gary Guo as collaborators on my fork. I do hope to get confirmation from him because he understands this edge condition better than I do, I think.
Co-authored-by: Ralf Jung <post@ralfj.de>
src/linkage.md
Outdated
where linking is done by the system runtime without `rustc` being involved. | ||
|
||
r[link.unwinding.potential] | ||
A Rust binary or `staticlib` is called *potentially unwinding* if any of the following conditions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a name for "the final artifact of Rust linkage, with a single Rust runtime included"? Always saying "Rust binary or staticlib
" is awkward...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this also needs to account for cdylib
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we really need a proper term for this, not an enumeration. "final Rust artifact"? Or can we say "Rust binary" since staticlib
and cdylib
are binaries but in some sense rlib is not? Not sure if that makes any sense...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bjorn3 maybe you have a good idea for what to call this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a great name for it either unfortunately. bin, dylib, cdylib and proc-macro are all linked crates, but staticlib isn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to just directly talk about artifacts and whether they share the same copy of the Rust runtime.
@traviscross @ehuss @chorman0773 I think that resolves all concerns, though it wouldn't hurt if someone could read over this to make sure it is still coherent. |
r[link.unwinding.potential] | ||
A Rust artifact is called *potentially unwinding* if any of the following conditions is met: | ||
- The artifact is linked with [the `panic=unwind` runtime][panic-runtime]. | ||
- The artifact contains a crate built with `-Cpanic=unwind` that makes a call | ||
to a function using a `-unwind` ABI. | ||
- The artifact makes a `"Rust"` ABI call to code running in another Rust | ||
artifact that has a separate copy of the Rust runtime, and that other artifact is | ||
potentially unwinding. | ||
|
||
> [!NOTE] | ||
> This definition captures whether a `"Rust"` ABI call inside a Rust artifact can ever | ||
> unwind. | ||
|
||
r[link.unwinding.prohibited] | ||
If a Rust artifact is potentially unwinding, then all its crates must be built with `-Cpanic=unwind`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nbdd0121 could you fact-check this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks correct to me.
@rustbot labels +I-lang-nominated Given all the attention and iteration this has received, we should probably re-review this en banc on the lang side to confirm. |
Tracking issue: rust-lang/rust#74990